Skip to content

Candidate for 6.0.5 release - #2416

Open
kushti wants to merge 68 commits into
masterfrom
v6.0.5
Open

Candidate for 6.0.5 release #2416
kushti wants to merge 68 commits into
masterfrom
v6.0.5

Conversation

kushti and others added 30 commits January 28, 2026 12:09
…e-id-length

Validate Transaction API box and token id lengths
…-hex

Reject invalid secret proof hint hex
Add Inv -> RequestModifier test to ErgoNodeViewSynchronizerSpecificat…
Remove incorrect security annotations from public mining and script API routes
a-shannon and others added 18 commits July 20, 2026 02:56
…-ids

Prevent duplicate IDs in OrderedTxPool
Fix indentation in openapi.yaml for CommitmentWithSecret
Block candidate generation improvements
Preserve extra index consistency across chain switches
Log method, relative URI, response status and elapsed time for every query served by the node's HTTP interface. Bodies are not logged: requests to this API carry secrets (mnemonic on /wallet/restore, password on /wallet/unlock).

Logging goes through ScorexLogging rather than akka's LoggingAdapter, so no dependency is added and the node's HTTP verbosity is not tied to akka's global log level. It is off by default, as the root logger is at INFO, and costs nothing when off since log.debug is a macro guarded by isDebugEnabled.

The directive wraps the route outside handleRejections, so rejected requests are logged too, with the status they were answered with.

Closes #1909
Commented-out logger element so the switch is discoverable.
Attaches a logback ListAppender to the service logger and asserts on what is emitted: one line per served query with method, URI, status and duration; the query string included and unmatched paths logged with the status they were answered with; nothing logged below DEBUG; and the response body unchanged with logging on and off.

@jozanek jozanek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full branch against master, focused on protocol/wire compatibility, concurrency, DoS surface, and test coverage.

No protocol blockers: serializers untouched, stricter NiPoPoW validation is bootstrap-only with prover/verifier symmetry intact, FullBlockApplied.txIds never hits the wire, and the outbound buffer cap changes no message format. Test coverage is excellent — every fix ships a regression spec.

Findings below: 3 minor, 5 nit — release-note/visibility items, no code defects.

*
*/
case class PoPowParams(m: Int, k: Int, continuous: Boolean)
final class PoPowParams private (val m: Int, val k: Int, val continuous: Boolean, val minChainLength: Int)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR: PoPowParams going from case class to a private-constructor class with apply returning Try is a source/binary break in ergo-core's public API (no more direct construction, copy, or unapply). Since ergo-core is the library SPV clients build against, this deserves an explicit entry in the 6.0.5 release notes.


def apply(m: Int, k: Int, continuous: Boolean): Try[PoPowParams] = Try {
require(isValid(m, k), s"Invalid NiPoPoW parameters: m=$m, k=$k")
new PoPowParams(m, k, continuous, m + k)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: minChainLength is computed and stored but never read in production code (only one test asserts it). Either drop it or use it in prove's chain.lengthCompare(k + m) check so it earns its place.


private val mempoolCapacity = settings.nodeSettings.mempoolCapacity

private def withoutTransaction(id: ModifierId): TreeMap[WeightedTxId, UnconfirmedTransaction] = {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR: the self-heal fallbacks (withoutTransaction's full filter, hasUnregisteredTransaction, currentTransaction's orElse scan) are O(n) per mutation once orderedTransactions.size != transactionsRegistry.size, so a corrupted pool under tx flood pays O(n) per admission until healed. The healthy path keeps O(log n) via the size-equality guard, so this is fine as a recovery path — but consider logging when the degraded path triggers, so pool corruption is visible in production instead of silently costing CPU.

val elapsed = System.currentTimeMillis() - stats.startMeasurement
if (stats.takenTxns != 0) {
elapsed * posInPool / stats.takenTxns
val cappedElapsed = math.max(0L, math.min(elapsed, MemPoolStatistics.measurementIntervalMsec.toLong))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: MemPoolStatistics.measurementIntervalMsec = 60 * 1000 is commented "one hour" but is one minute. Pre-existing, but the new elapsed-time cap here now depends on this constant, so worth fixing the comment while in the area.

MaxMessageSize.toLong + HeaderLength + ChecksumLength

// Independently bound collection overhead from small messages.
private[network] val MaxBufferedOutboundMessages: Int = 64

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: MaxBufferedOutboundMessages = 64 and the byte cap are hard-coded. If field tuning ever turns out to be needed (e.g. peers on high-latency links tripping the abort), exposing them under scorex.network would avoid a redeploy — fine to defer.

val modifierIdGet: Directive1[ModifierId] = parameters("id".as[String])
.flatMap(handleModifierId)

private def parseModifierId(value: String): Try[ModifierId] =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR: client-visible behavior change worth a release-note entry: modifier/box/token ids with wrong byte length and scan ids outside Short range now return 400 where they were previously accepted, silently truncated (scanIdInt.toShort querying the wrong scan!), or 500'd. Good hardening — just make sure API consumers hear about the stricter validation.

} ~
(path("openapi.yaml") & get) {
getFromResource("api/openapi-ai.yaml", ContentTypes.`text/plain(UTF-8)`)
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: removal of /openapi.yaml and /.well-known/ai-plugin.json is a deliberate feature removal, but anyone who scripted against those endpoints will notice — one line in the release notes would cover it.

.withFallback(nodeSeedConfigs.head)
.withFallback(allowLocalConfig)

// `lazy` so the container is only started when a test actually touches `node`.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: the lazy val change is right, but it documents that the only OpenAPI conformance test remains ignored (checker image gone) — so the openapi.yaml edits on this branch aren't machine-checked. Worth a tracking issue to restore an OpenAPI validation step.

@jozanek jozanek left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the 6.0.5 release candidate

What "Request changes" means here: GitHub will show this PR as blocked on this review until it is re-reviewed or dismissed. I am using it because of one MAJOR functional finding — the extra indexer can stall permanently after the new catch-up deferral (inline below) — which should be fixed, or explicitly accepted, before the release is tagged. It is not a protocol or consensus objection.

Scope: the full PR diff against master — mempool duplicate-id fix, candidate-generator improvements, extra-indexer reorg handling, API input validation and openapi corrections, AI-plugin removal, NiPoPoW parameter/PoW validation, p2p outbound-buffer cap and blacklist cleanup, wallet burn-order fix, mempool fee/wait-time clamps, and the API query logger.

Protocol screen — why there are no blockers:

  • No serializer byte-format changes anywhere. The NiPoPoW hardening is verifier-side only: the deserializer still parses the same byte shapes (the new tests round-trip proofs with m = 0 and only isValid rejects them), and the tightened isValid (param sanity + per-header Autolykos PoW) only rejects proofs an honest prover never produces, so prover/verifier symmetry is retained.
  • LocalBlockApplied/RemoteBlockApplied gained a txIds field, but these are internal event-stream messages published by ErgoNodeViewHolder; they are never serialized to the network.
  • The appVersion = 6.0.5 handshake bump is the standard release procedure, and the openapi/do-release.sh version stamps are consistent with it.
  • The outbound-buffer cap and blacklist changes alter connection management only, not the wire format.

Also verified along the way: the removed openapi security annotations now match the code (only candidateWithTxs carries withAuth in MiningApiRoute; ScriptApiRoute has none), the old chainSlice range guard was dead code so the new check closes a real unbounded-request hole, and the scan-id .toShort truncation fix stops queries like 70000 from silently returning scan 4464's data.

Findings: 1 MAJOR, 5 MINOR, 3 NIT — all inline.

context.become(receive.orElse(loaded(newState)))
self ! Index()
} else {
log.info("Deferring catch-up because the next header does not extend the indexed tip")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MAJOR The deferral branch stops the Index() self-loop without scheduling any retry, and while caughtUp = false the actor has no handler for FullBlockApplied (the handler at line 501 requires caughtUp), so the only signal that can resume indexing is a Rollback event.

Consider a near-tip headers-only fork that briefly becomes the best header chain but whose full blocks never win (the losing side of a miner race): the guard at line 481 sees a next header that does not extend the indexed tip and defers. If the original chain then outgrows the fork, the best header chain flips back — but no Rollback is ever published, because the full-block chain never switched. The indexer stays stalled until node restart, silently dropping every subsequent FullBlockApplied.

Suggestion: add case _: FullBlockApplied if !state.caughtUp && !state.rollbackInProgress => self ! Index() so every applied block re-evaluates the deferral condition (or re-schedule Index() with a short delay instead of only logging).

if (modCount >= saveLimit) saveProgress(newState)
context.become(receive.orElse(loaded(newState)))
self ! Index()
val nextHeaderOpt = history.bestHeaderAtHeight(state.indexedHeight + 1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR During catch-up this adds a bestHeaderAtHeight(h + 1) (heightIds index read + full header fetch) per block, and index() then re-reads bestHeaderIdAtHeight(height) at line 381 because headerOpt is None on this path — two redundant storage reads per block on the full-reindex hot path, where they multiply across millions of blocks.

Suggestion: pass the already-fetched nextHeaderOpt into index(state.incrementIndexedHeight, nextHeaderOpt) so both the parent check and indexedHeaderId reuse a single read.

))
})
} ~
(path(".well-known" / "ai-plugin.json") & get) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Removing /openapi.yaml and /.well-known/ai-plugin.json is clearly intentional (ChatGPT-plugin retirement), but it is a breaking removal of public endpoints — anything still fetching them gets a 404 after upgrade. Worth an explicit line in the 6.0.5 release notes.

// `lazy` so the container is only started when a test actually touches `node`.
// The single test below is currently `ignore`d (the openapi-checker image is gone),
// so without `lazy` we would start and tear down a node for nothing.
lazy val node: Node = docker.startDevNetNode(offlineGeneratingPeer).get

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR The spec's only test remains ignored (the openapi-checker image is gone), so this suite passes CI while providing zero signal — the new comment documents the situation but keeps the dead spec. Consider deleting the spec or reviving the check with a maintained validator image.

*
*/
case class PoPowParams(m: Int, k: Int, continuous: Boolean)
final class PoPowParams private (val m: Int, val k: Int, val continuous: Boolean, val minChainLength: Int)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR minChainLength is not read anywhere in production code — its only consumer is the assertion in PoPowAlgosSpec. If it is groundwork for the follow-up NiPoPoW parsing work (#2461), fine to keep, but then a short comment saying so would help; otherwise it is a dead field that suggests a validation which does not actually happen yet.

suffixHead.checkInterlinksProof()
}

lazy val hasValidPow: Boolean = headersChain.forall(popowAlgos.hasValidPow)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MINOR Validating every header's Autolykos PoW is the right fix, but isValid is evaluated inside ErgoNodeViewSynchronizer's receive (case Success(proof) if proof.isValid around line 1085 of ErgoNodeViewSynchronizer.scala), so a proof chain of hundreds of headers now runs full PoW verification on the synchronizer's dispatcher thread, stalling its mailbox during nipopow bootstrap — and several proofs can arrive back-to-back from the p2pNipopows peers.

Suggestion: run the proof validation in a Future on a dedicated dispatcher and pipeTo the result back, keeping the synchronizer responsive.

private val mempoolCapacity = settings.nodeSettings.mempoolCapacity

private def withoutTransaction(id: ModifierId): TreeMap[WeightedTxId, UnconfirmedTransaction] = {
// Keep healthy mutations logarithmic; scan by ID only after cardinality diverges.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT The size-equality heuristic takes the fast path on compensating corruption — one duplicate key plus one orphaned entry leaves the sizes equal, so a duplicate would survive withoutTransaction. Fine as best-effort self-healing, but worth extending the comment to note that limitation.

done.await()
awaitCondition(done)
indexer ! GenerateBetterChainTip()
lock.lock()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT awaitCondition fixes the lock discipline (the old pattern never unlocks after await()), but this test still uses bare lock.lock(); created.await() in two places. Worth finishing the migration to awaitCondition(created) here too.


// Keep one maximum serialized frame per peer. Backpressured snapshot transfers
// retry instead of retaining their entire application-level in-flight window.
private[network] val MaxBufferedOutboundBytes: Long =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT Question on honest-path headroom: the byte cap is one max frame (~16.4 MB), which the tests show fits 4 in-flight snapshot chunks — but a peer that requested a large block batch can legitimately have several Modifiers responses (up to ~8.4 MB each) queued while its socket is stalled, and two of those already exceed the cap, aborting the connection. Reconnect makes this self-healing, so it may well be acceptable — but worth confirming the serving side never queues more than one large response per request round, or noting that connection churn under full-stall is the intended trade-off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants